-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commonjs support with import_style option #211
Conversation
vars["filename"] = filename; | ||
|
||
printer->Print(vars, "const proto = {};\n"); | ||
if(!package.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a <space>
after each if
, while
, and switch
for consistency please?
You might have to rebase this as well. Thanks |
1554679
to
17abe5b
Compare
@@ -290,16 +323,22 @@ class GrpcCodeGenerator : public CodeGenerator { | |||
|
|||
string file_name; | |||
string mode; | |||
string import_style_str; | |||
ImportStyle import_style; | |||
|
|||
for (uint i = 0; i < options.size(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stanley-cheung The uint
is causing an error for me. Should this not be size_t
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right. Sorry please change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
17abe5b
to
ad7f83a
Compare
} | ||
} | ||
|
||
printer->Print(vars, "proto.$package_name$ = require('./$filename$_pb.js');\n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the proto messages classes have to be generated into one single file named [some name]_pb.js, and can't be generated into one file per message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm matching how the protoc compiler outputs it's message with import_style set to commonjs. They output a js file per proto file with the name <proto-file-name>_pb.js
.
@zaucy Do you mind doing a rebase here? I am still trying to run the full npm + commonjs example. Just to make sure: you are using the same Echo backend gRPC c++ server + Envoy proxy right? So the only thing I need to swap out is how the frontend HTML calls the JS code right? I suppose one easy way is just set my webserver to point to a directory with index.js + node_modules/ generated from PR #210? |
ad7f83a
to
34450f2
Compare
@stanley-cheung Done! Yeah all you should have to do is change how the frontend imports the client code. You'll need a frontend that works with commonjs imports. Anything based off webpack would work. Do you want me to write an example in webpack, angular and react? I can do that in another PR early next week. |
@zaucy Yea such a complete example would greatly benefit the project! Thanks again for your great contributions! |
Ha, I was able to (well, use a very roundabout way) finally verify that the CommonJS generated code works end-to-end! Hack 1: I put all the generated code Hack 2: This line in the grpc-web generated code doesn't work But because I ran And then I make
And then And finally an
Hack 3: |
@zaucy I noticed a potential discrepancy on where the generated file is placed for the grpc plugin vs the normal (message) protoc JS plugin: Let's say my protoc command is like this:
The grpc generated file is placed in Should they be both placed in the But then I do noticed that in
So it does work as is. But it feels weird that the generated files are in a different location. So perhaps it makes sense to 1. place both files into |
Off the top of my head generators for other languages (I've worked with Python and C# at least, maybe C++) put all output relative to where the command is run, it this case And the reasoning is simple: if you want it in the same place as the |
@stanley-cheung I'll take a look later today and make sure it's consistent. |
@lalomartins sorry I was a little ambiguous. The One is |
Ah, I see what you mean. That's especially important if there are multiple files and they're not all in the same place (like you have a subdirectory — we had a layout like this in a recent project). |
Nevermind, I can manipulate the
|
* grpcClient.finishSend() is essential for websocket transport * call hackIntoNormalGrpcRequest() in handleWebSocket()
Created for #197
Works in tandem with #210